Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serve JS dependency files locally #186

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Serve JS dependency files locally #186

merged 3 commits into from
Oct 29, 2021

Conversation

jvfe
Copy link
Contributor

@jvfe jvfe commented Oct 16, 2021

  • Changes Pyodide Javascript file from a CDN to a local file. Should help moving towards the editor being usable offline.
    I believe this is the only JS/CSS dependency that wasn't being served locally, but I might've missed something.
  • Closes To serve assets + pyodide JS/CSS files locally #157

Now, from what I can see, the only files being loaded over a connection are the Pyodide WASM files that get loaded from here:

indexURL : "https://pyodide-cdn2.iodide.io/v0.18.1/full/",

I'm not really sure how to proceed with these, since they are data/binary files and not JS files.

@jvfe
Copy link
Contributor Author

jvfe commented Oct 16, 2021

Oh, I was also wondering if this line is still necessary:

<link rel="stylesheet" href="/static/styles/highlightjs.min.css">

Since this highlightjs.min.css file doesn't exist in the project, it just returns a 404 when loading a sketch so I'm removing it.

Copy link
Owner

@berinhard berinhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing invalid url for the CSS file @jvfe.

About the WASM files, I think we can serve them too. The indexURL from Pyodide is just a string prefix used to built the complete URL to both pyodide.asm.js and pyodide.asm.data. So, maybe just using one of the following should work:

{indexURL: "/static/js/pyodide/"} 
// or even
{indexURL: "http://localhost:5000/static/js/pyodide/"} 

Can you try that?

@berinhard
Copy link
Owner

@jvfe also, I update the Github workflow so CI can run every new PR (see ef3ba21). Can you update your PR with develop branch so CI build can get triggered?

@jvfe
Copy link
Contributor Author

jvfe commented Oct 18, 2021

Thanks for removing invalid url for the CSS file @jvfe.

About the WASM files, I think we can serve them too. The indexURL from Pyodide is just a string prefix used to built the complete URL to both pyodide.asm.js and pyodide.asm.data. So, maybe just using one of the following should work:

{indexURL: "/static/js/pyodide/"} 
// or even
{indexURL: "http://localhost:5000/static/js/pyodide/"} 

Can you try that?

Hi, the first one worked, though I had to add a few other files too or else there would be some 404s. But seems to be working fine offline now. Thank you for your help.

I rebased and pushed with the new updates, so the CI should be running.

Copy link
Owner

@berinhard berinhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jvfe for the work here. The local pyodide works smoothly with the local server, but the "pure web" version of it is breaking due to 404 because the pyp5js new and neither compile one copies the pyodide content to the sketche's static dir. This the <script src="/static/js/pyodide/pyodide_v0.18.1.js"></script> returns a 404.

Anyway, I don't think we should change this web version. This one should rely on CDN versions as most as possible.

I think the change here is more complex than what I predicted. Possibly we'll have to:

  • Update pyodide index.html template to use a {{ pyodide_js_url }}
  • Update pyodide target_sketch.js.template template to use a {{ pyodide_jindex_url }}

Maybe moving such configurations to the .properties.json file can be a good strategy. If this is to complex or to big, I tackle that or even pair program with you on it. Let me know what do you think about it.

But, again, thanks for starting with this =)

@berinhard
Copy link
Owner

@jvfe I spent some time on this and it can become more problematic. I'll work on it later this Thursday to make this feasible.

@jvfe
Copy link
Contributor Author

jvfe commented Oct 26, 2021

Thanks @jvfe for the work here. The local pyodide works smoothly with the local server, but the "pure web" version of it is breaking due to 404 because the pyp5js new and neither compile one copies the pyodide content to the sketche's static dir. This the <script src="/static/js/pyodide/pyodide_v0.18.1.js"></script> returns a 404.

Anyway, I don't think we should change this web version. This one should rely on CDN versions as most as possible.

I think the change here is more complex than what I predicted. Possibly we'll have to:

* [ ]  Update pyodide `index.html` template to use a `{{ pyodide_js_url }}`

* [ ]  Update pyodide `target_sketch.js.template` template to use a `{{ pyodide_jindex_url }}`

Maybe moving such configurations to the .properties.json file can be a good strategy. If this is to complex or to big, I tackle that or even pair program with you on it. Let me know what do you think about it.

But, again, thanks for starting with this =)

Oh wow, my mistake for not testing the pure web version. So should I revert all changes in templates/pyodide/ and keep only those in http/templates/view_sketch.html? I don't have much time this week but I'd be up for pair programming with you, since I'm not 100% sure what the problem is at the moment.

@jvfe I spent some time on this and it can become more problematic. I'll work on it later this Thursday to make this feasible.

No problem, I'll wait on your input.

@berinhard berinhard merged commit 1ac0340 into berinhard:develop Oct 29, 2021
@berinhard
Copy link
Owner

@jvfe I've merged this PR and started a new one to implement the required tooling to be able to control if the user will use CDN or will host pyodide by their own. The PR with all the changes is #191 and there's a single commit grouping all the TODO notes (d72f0f2) about how to make the same CDN/local logic to work with pyodide too.

Thanks for starting with this work!

@jvfe jvfe deleted the fix/issue_157 branch November 2, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

To serve assets + pyodide JS/CSS files locally
2 participants